Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Text 2d alignment fix #17365

Merged
merged 2 commits into from
Jan 20, 2025
Merged

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Jan 14, 2025

Objective

Text2d ignores TextBounds when calculating the offset for text aligment.
On main a text entity positioned in the center of the window with center justification and 600px horizontal text bounds isn't centered like it should be but shifted off to the right:
hellox
(second example in the testing section below)

Fixes #14266

I already had a PR in review for this (#14270) but it used post layout adjustment (which we want to avoid) and ignored TextBounds.

Solution

  • If TextBounds are present for an axis, use them instead of the size of the computed text layout size to calculate the offset.
  • Adjust the vertical offset of text so it's top is aligned with the top of the texts bounding rect (when present).

Testing

use bevy::prelude::*;
use bevy::color::palettes;
use bevy::sprite::Anchor;
use bevy::text::TextBounds;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .run();
}

fn example(commands: &mut Commands, dest: Vec3, justify: JustifyText) {
    commands.spawn((
        Sprite {
            color: palettes::css::YELLOW.into(),
            custom_size: Some(10. * Vec2::ONE),
            anchor: Anchor::Center,
            ..Default::default()
        },
        Transform::from_translation(dest),
    ));

    for a in [
        Anchor::TopLeft,
        Anchor::TopRight,
        Anchor::BottomRight,
        Anchor::BottomLeft,
    ] {
        commands.spawn((
            Text2d(format!("L R\n{:?}\n{:?}", a, justify)),
            TextFont {
                font_size: 14.0,
                ..default()
            },
            TextLayout {
                justify,
                ..Default::default()
            },
            TextBounds::new(300., 75.),
            Transform::from_translation(dest + Vec3::Z),
            a,
        ));
    }
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2d::default());

    for (i, j) in [
        JustifyText::Left,
        JustifyText::Right,
        JustifyText::Center,
        JustifyText::Justified,
    ]
    .into_iter()
    .enumerate()
    {
        example(&mut commands, (300. - 150. * i as f32) * Vec3::Y, j);
    }

    commands.spawn(Sprite {
        color: palettes::css::YELLOW.into(),
        custom_size: Some(10. * Vec2::ONE),
        anchor: Anchor::Center,
        ..Default::default()
    });
}
cap

This probably looks really confusing but it should make sense if you imagine each block of text surrounded by a 300x75 rectangle that is anchored to the center of the yellow square.

use bevy::prelude::*;
use bevy::sprite::Anchor;
use bevy::text::TextBounds;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2d::default());

    commands.spawn((
        Text2d::new("hello"),
        TextFont {
            font_size: 60.0,
            ..default()
        },
        TextLayout::new_with_justify(JustifyText::Center),
        TextBounds::new(600., 200.),
        Anchor::Center,
    ));
}
hello

The text being above the center is intended. When TextBounds are present, the text block's offset is calculated using its TextBounds not the layout size returned by cosmic-text.

Probably we should add a vertical alignment setting for Text2d. Didn't do it here as this is intended for a 0.15.2 release.

@ickshonpe ickshonpe added C-Bug An unexpected or incorrect behavior A-Text Rendering and layout for characters S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 14, 2025
@rparrett rparrett self-requested a review January 14, 2025 16:42
Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@BenjaminBrienen BenjaminBrienen added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Jan 15, 2025
@rparrett rparrett added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 17, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 20, 2025
Merged via the queue into bevyengine:main with commit 3f99a3e Jan 20, 2025
35 checks passed
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
# Objective

`Text2d` ignores `TextBounds` when calculating the offset for text
aligment.
On main a text entity positioned in the center of the window with center
justification and 600px horizontal text bounds isn't centered like it
should be but shifted off to the right:
<img width="305" alt="hellox"
src="https://github.com/user-attachments/assets/8896c6f0-1b9f-4633-9c12-1de6eff5f3e1"
/>
(second example in the testing section below)

Fixes bevyengine#14266

I already had a PR in review for this (bevyengine#14270) but it used post layout
adjustment (which we want to avoid) and ignored `TextBounds`.

## Solution

* If `TextBounds` are present for an axis, use them instead of the size
of the computed text layout size to calculate the offset.
* Adjust the vertical offset of text so it's top is aligned with the top
of the texts bounding rect (when present).

## Testing

```
use bevy::prelude::*;
use bevy::color::palettes;
use bevy::sprite::Anchor;
use bevy::text::TextBounds;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .run();
}

fn example(commands: &mut Commands, dest: Vec3, justify: JustifyText) {
    commands.spawn((
        Sprite {
            color: palettes::css::YELLOW.into(),
            custom_size: Some(10. * Vec2::ONE),
            anchor: Anchor::Center,
            ..Default::default()
        },
        Transform::from_translation(dest),
    ));

    for a in [
        Anchor::TopLeft,
        Anchor::TopRight,
        Anchor::BottomRight,
        Anchor::BottomLeft,
    ] {
        commands.spawn((
            Text2d(format!("L R\n{:?}\n{:?}", a, justify)),
            TextFont {
                font_size: 14.0,
                ..default()
            },
            TextLayout {
                justify,
                ..Default::default()
            },
            TextBounds::new(300., 75.),
            Transform::from_translation(dest + Vec3::Z),
            a,
        ));
    }
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2d::default());

    for (i, j) in [
        JustifyText::Left,
        JustifyText::Right,
        JustifyText::Center,
        JustifyText::Justified,
    ]
    .into_iter()
    .enumerate()
    {
        example(&mut commands, (300. - 150. * i as f32) * Vec3::Y, j);
    }

    commands.spawn(Sprite {
        color: palettes::css::YELLOW.into(),
        custom_size: Some(10. * Vec2::ONE),
        anchor: Anchor::Center,
        ..Default::default()
    });
}
```

<img width="566" alt="cap"
src="https://github.com/user-attachments/assets/e6a98fa5-80b2-4380-a9b7-155bb49635b8"
/>

This probably looks really confusing but it should make sense if you
imagine each block of text surrounded by a 300x75 rectangle that is
anchored to the center of the yellow square.

# 

```
use bevy::prelude::*;
use bevy::sprite::Anchor;
use bevy::text::TextBounds;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2d::default());

    commands.spawn((
        Text2d::new("hello"),
        TextFont {
            font_size: 60.0,
            ..default()
        },
        TextLayout::new_with_justify(JustifyText::Center),
        TextBounds::new(600., 200.),
        Anchor::Center,
    ));
}
```

<img width="338" alt="hello"
src="https://github.com/user-attachments/assets/e5e89364-afda-4baa-aca8-df4cdacbb4ed"
/>

The text being above the center is intended. When `TextBounds` are
present, the text block's offset is calculated using its `TextBounds`
not the layout size returned by cosmic-text.

# 

Probably we should add a vertical alignment setting for Text2d. Didn't
do it here as this is intended for a 0.15.2 release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Text Rendering and layout for characters C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JustifyText::Center doesn't place a text in the center of Text2dBounds
5 participants